Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Feat] Add image loading indicator #8238

Merged
merged 9 commits into from
Apr 3, 2022
Merged

[Feat] Add image loading indicator #8238

merged 9 commits into from
Apr 3, 2022

Conversation

mdneyazahmad
Copy link
Contributor

@mdneyazahmad mdneyazahmad commented Mar 19, 2022 •
edited by mountiny
Loading

Details

Adds an image loading indicator when images loads in chat and when an image viewer is opened.

Fixed Issues

$ #7905
$ #7584

Tests

  1. Open any chat
  2. Verify that when images loads it as a loading indicator.
  3. Click on an image (better if large size)
  4. Verify that a loading spinner appears when the image is being downloaded
  • Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. “toggleReport” and not “onIconClick”)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct english, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
    • I verified any copy / text shown in the product was added in all src/languages/* files
    • I verified any copy / text that was added to the app is correct english and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY
  • I verified any variables that can be defined as constants (ie. in CONST.js) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • Any functional components have the displayName property
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • Any internal methods are bound to “this” properly so there are no scoping issues
    • Any internal methods bound to “this” are necessary to be bound
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose and it is
  • If a new CSS style is added I verified that:
    • A similar style doesn’t already exist
    • The style can’t be created with an existing StyleUtils function
      (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

PR Reviewer Checklist

  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. “toggleReport” and not “onIconClick”).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct english, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
    • I verified any copy / text shown in the product was added in all src/languages/* files
    • I verified any copy / text that was added to the app is correct english and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY
  • I verified any variables that can be defined as constants (ie. in CONST.js) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • Any functional components have the displayName property
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • Any internal methods are bound to “this” properly so there are no scoping issues
    • Any internal methods bound to “this” are necessary to be bound
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
  • If a new CSS style is added I verified that:
    • A similar style doesn’t already exist
    • The style can’t be created with an existing StyleUtils function
      (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

QA Steps

  1. Open any chat
  2. Verify that when images loads it as a loading indicator.
  3. Click on an image (better if large size)
  4. Verify that a loading spinner appears when the image is being downloaded
  5. Verify that it does not keeps loading, finally it has to stop.
  • Verify that no errors appear in the JS console

Screenshots

Web

web.mp4

Mobile Web

mweb.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

android.mp4

@mdneyazahmad mdneyazahmad requested a review from a team as a code owner March 19, 2022 11:27
@mdneyazahmad
Copy link
Contributor Author

This pr is slightly different from my initial proposal. I am working on a very similar issue and found a performance related issue #8203 (comment) This pr includes that fix.

@mountiny mountiny mentioned this pull request Mar 20, 2022
Merged
86 tasks
@mountiny
Copy link
Contributor

@mdneyazahmad As mentioned in #8203 (comment), would you be able to update this PR with the code from the other one? I would revert the previous one as we dont want to push the problem to staging.

Let's make sure it works here thoroughly

@mountiny mountiny mentioned this pull request Mar 20, 2022
Closed
@mountiny
Copy link
Contributor

I have updated the body of this PR to also sin #7584 issue. Let's update the code with the lines needed from reverted PR and then test it all here. The features are similar (same).

Also dont forget to update the QA and test steps please to reflect that.

Thank you very much!

mountiny
mountiny reviewed Mar 20, 2022
View reviewed changes
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, these changes so far look good to me, just as I mentioned, we will need to add the previous PR with the imageLoadingStart as class functions.

Thanks!

@mdneyazahmad
Copy link
Contributor Author

@mountiny I will update it with that fix too. Thank you!

@mdneyazahmad
Copy link
Contributor Author

@mountiny done

@mountiny
Copy link
Contributor

Thanks!

@parasharrajat just to clarify, I have asked @mdneyazahmad to include changes from #7584 where we had to revert the PR. These changes are very similar, so it made sense to include them together in one PR which is still manageable and testeable.

@parasharrajat
Copy link
Member

I am fine with it @mountiny .

parasharrajat
parasharrajat suggested changes Mar 21, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Are you following rules from the checklist? Please carefully follow the checklist. I know this could be an extra effort but reviewers also follow the same. Not following it and putting up the PR for review, will indicate a lack of testing. You have already worked on a few issues, we expect you to be more proactive. Thanks.

parasharrajat
parasharrajat suggested changes Mar 21, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of duplicate code across components. Let's dry this up. Thanks.

@mdneyazahmad
Copy link
Contributor Author

@parasharrajat I really wanted to discuss the refactoring Image component as it is used at many places. I want to create a wrapper component for Image that manages loading indicator internally.

@parasharrajat
Copy link
Member

Sure, sounds good.

@AndrewGable AndrewGable removed the request for review from a team March 21, 2022 22:23
@mdneyazahmad
Copy link
Contributor Author

Updated

@mdneyazahmad
Copy link
Contributor Author

I did not create the wrapper component to manage loading state internally because this component will be used at many places and that will not be much useful we still have to manage the loading state and disable user interaction (modal).

parasharrajat
parasharrajat suggested changes Mar 23, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can still see one cleanup.

Move the whole loader thing with Image into the component and then just use it everywhere needed. The name could be ImageWithLoader.

@mountiny
Copy link
Contributor

@parasharrajat seems like your specific comment was not submitted.

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 30, 2022 •
edited
Loading

Sure thing! On it

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 30, 2022 •
edited
Loading

Bug: Loading indicator disappears when going from offline to online in attachment view.

  1. Go offline.
  2. Open an image that isn't cached (opening first time)
    We'll see a loading indicator as expected.
  3. Go online.
  4. The loading indicator disappears! And we have to reopen the attachment view to load the image.

@mdneyazahmad please let me know if you can repro this, and if the steps are clear.

Screen.Recording.2022-03-30.at.10.13.33.PM.mov

@mountiny
Copy link
Contributor

@rushatgabhane Great testing!

@mdneyazahmad As this is a new feature, we will try to make sure to test this as much as possible. I realize this is taking longer now, so I would be fine with doubling the reward for one of the linked issues 👍

Lets get this bug fixed and make sure everything tested well

@mdneyazahmad
Copy link
Contributor Author

I tested with the steps, but the behaviour is different in my case. At step 4 the loading spinner does not go away have to reopen in order to work (may be due to my internet connection is usually slow). The only condition the spinner go away is either success or failed. In your case it seems to be the latter case and we do not have any logic to handle that case yet. I also see that the production website (without this pr) does not load images at step 4.

I realize this is taking longer now

I do not see this issue. Could you please confirm it? I create a log in the imageLoadingStart and it is calling 1 time as expected. It means, it is downloading it just once.

cc: @rushatgabhane @mountiny

@parasharrajat
Copy link
Member

parasharrajat commented Apr 1, 2022 •
edited
Loading

The loading indicator disappears! And we have to reopen the attachment view to load the image.

IMO, this is not part of this PR. @rushatgabhane you should report this on slack. we should show an inline message to indicate that the image load failed.

The loader should stop either when the image is loaded or failed to load. Continuous loader is a bug.

@rushatgabhane
Copy link
Member

we should show an inline message to indicate that the image load failed.

Okay, I'll start a discussion on slack on how to handle failed pessimistic request for an image. I agree that this should be done in a different issue.

@rushatgabhane

This comment was marked as off-topic.

Sign in to view
@parasharrajat
Copy link
Member

that will not be the case always. We don't always have a thumbnail. And web always needs to fetch the image from the URL. Until we have offline images, we can't do anything. Also, I am totally against showing a low-resolution thumbnail as fullscreen. If you want this, then images should be progressive.

@rushatgabhane
Copy link
Member

Also, I am totally against showing a low-resolution thumbnail as fullscreen

Okay, I see that Tim is against this too.
Alright then, I'm marking my comments as off topic

@mdneyazahmad
Copy link
Contributor Author

The loader should stop either when the image is loaded or failed to load. Continuous loader is a bug.

On slow connection it takes time to download the image and it does not trigger onEndLoad. If it has timeout it would trigger onError after timeoutDuration and then we display the error or give them a button to reload the image.

@rushatgabhane
Copy link
Member

If it has timeout it would trigger onError after timeoutDuration

@mdneyazahmad sorry, I don't understand.
Isn't that the same thing as failed request?

If yes, I think it's best that we handle it in a seperate issue

@mdneyazahmad
Copy link
Contributor Author

@rushatgabhane Image component does not have this feature. That was the flow I am suggesting.

rushatgabhane
rushatgabhane approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member

@rushatgabhane rushatgabhane left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mountiny LGTM!

The bug that I mentioned earlier should be dealt in a different issue. Because it's a part of offline UX discussion

(I've started a discussion for this here)

@mountiny
Copy link
Contributor

mountiny commented Apr 3, 2022

@rushatgabhane @parasharrajat @mdneyazahmad Thank you very much for the discussion here and handling it, I was ooo finishing bachelor thesis 🥵

I agree that showing low resolution picture on fullscreen is big ⛔

And I am happy to sort the slow connection loadings as part of the Offline project pending this PR does not introduce any new problems we havent had before!

@mountiny
Copy link
Contributor

mountiny commented Apr 3, 2022

@mdneyazahmad Thank you for your patience on this feature, hopefully, all will work out this time!

@mountiny mountiny merged commit 9a8291b into Expensify:main Apr 3, 2022
@OSBotify
Copy link
Contributor

OSBotify commented Apr 3, 2022

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@rushatgabhane
Copy link
Member

bachelor thesis

@mountiny that makes the two of us 😫

@mountiny
Copy link
Contributor

mountiny commented Apr 3, 2022

@rushatgabhane You got this!

@OSBotify OSBotify mentioned this pull request Apr 5, 2022
Closed
22 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Apr 5, 2022

🚀 Deployed to staging by @mountiny in version: 1.1.51-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@melvin-bot melvin-bot bot mentioned this pull request Apr 6, 2022
Closed
@OSBotify
Copy link
Contributor

OSBotify commented Apr 6, 2022

🚀 Deployed to production by @roryabraham in version: 1.1.51-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Apr 6, 2022

🚀 Deployed to production by @roryabraham in version: 1.1.51-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 cancelled 🔪
🕸 web 🕸 success ✅

@SumitDiyora SumitDiyora mentioned this pull request Apr 20, 2022
Closed
@s77rt
Copy link
Contributor

s77rt commented Mar 3, 2023

This PR is partly responsible for a regression here #15288. The use of more than one callback is bugged (DylanVann/react-native-fast-image#975), in this PR it was the use of both onLoadStart and onLoadEnd where the execution of those two callbacks is not always in order.

@s77rt s77rt mentioned this pull request Mar 3, 2023
Closed
1 task
@sobitneupane
Copy link
Contributor

This PR has probably caused this regression.

Issue: Loading spinner is cut off on long images when reopening the chat

@sobitneupane sobitneupane mentioned this pull request May 24, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parasharrajat parasharrajat parasharrajat requested changes

@rushatgabhane rushatgabhane rushatgabhane approved these changes

@mountiny mountiny mountiny approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mdneyazahmad @mountiny @parasharrajat @rushatgabhane @OSBotify @s77rt @sobitneupane